fix(openai): recover from invalid reasoning signatures - #6392
Conversation
WalkthroughAdds a cached OpenAI reasoning fallback that removes encrypted reasoning content after an invalid signature, rebuilds sanitized pass-through requests, permits one immediate retry for Responses and ResponsesCompact modes, and exposes the behavior through channel settings. ChangesOpenAI reasoning signature fallback
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAI
participant Relay
participant ResponsesHelper
participant ReasoningFallback
OpenAI-->>Relay: thinking_signature_invalid
Relay->>ReasoningFallback: mark invalid signature
ReasoningFallback-->>Relay: enable one retry
Relay->>ResponsesHelper: prepare retry request
ResponsesHelper->>ReasoningFallback: remove encrypted reasoning content
ReasoningFallback-->>ResponsesHelper: sanitized input
ResponsesHelper->>OpenAI: retry request
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
relay/responses_handler.go (1)
110-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the redundant byte slice clone.
The
sjsonlibrary functions, includingsjson.SetRawBytes, do not mutate the input byte slice; they allocate and return a newly sized slice containing the modified JSON. Therefore, manually cloningjsonDatabeforehand is redundant and wastes memory, especially on large request payloads.♻️ Proposed fix
- // BodyStorage owns the returned bytes. Keep its original request body - // intact for later retries that may select a non-OpenAI API type. - jsonData = append([]byte(nil), jsonData...) jsonData, err = sjson.SetRawBytes(jsonData, "input", request.Input)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/responses_handler.go` around lines 110 - 113, Remove the redundant append-based byte slice clone in the response handling flow before sjson.SetRawBytes, while keeping the SetRawBytes update to the "input" field unchanged. Rely on SetRawBytes to return the newly allocated modified JSON slice and preserve the existing error handling and retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/relay.go`:
- Around line 235-243: Update the reasoningSignatureRetry branch in the relay
retry flow to increment retryLimit unconditionally whenever
shouldRetryOpenAIReasoningSignatureInvalid returns true. Remove the retry-count
condition while preserving the existing continue behavior and normal shouldRetry
path.
---
Nitpick comments:
In `@relay/responses_handler.go`:
- Around line 110-113: Remove the redundant append-based byte slice clone in the
response handling flow before sjson.SetRawBytes, while keeping the SetRawBytes
update to the "input" field unchanged. Rely on SetRawBytes to return the newly
allocated modified JSON slice and preserve the existing error handling and retry
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98ab15ce-c0f9-43fc-8168-5fa972612c69
📒 Files selected for processing (6)
controller/relay.gocontroller/relay_reasoning_retry_test.gorelay/responses_handler.goservice/openai_reasoning_fallback.goservice/openai_reasoning_fallback_test.gotypes/error.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/i18n/locales/fr.json`:
- Around line 5218-5219: Update the French translation for “Remove reasoning
encrypted_content and retry once when the upstream returns
thinking_signature_invalid” to explicitly preserve the nested field path
“reasoning.encrypted_content,” matching the actual field removed during
recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0583bb5-1867-44ad-b75a-ce86e6bbc339
📒 Files selected for processing (15)
controller/relay.gocontroller/relay_reasoning_retry_test.godto/channel_settings.gorelay/responses_handler.goweb/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/src/features/channels/lib/channel-form-errors.tsweb/src/features/channels/lib/channel-form.tsweb/src/features/channels/types.tsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/i18n/locales/zh.json
🚧 Files skipped from review as they are similar to previous changes (3)
- controller/relay_reasoning_retry_test.go
- relay/responses_handler.go
- controller/relay.go
| "Recover invalid reasoning signatures": "Récupérer les signatures de raisonnement invalides", | ||
| "Remove reasoning encrypted_content and retry once when the upstream returns thinking_signature_invalid": "Supprimer reasoning.encrypted_content et réessayer une fois lorsque le service en amont renvoie thinking_signature_invalid" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the exact reasoning.encrypted_content field path.
The French description currently says reasoning encrypted_content, which is ambiguous and does not match the actual nested field removed during recovery.
Suggested wording
- "Remove reasoning encrypted_content and retry once when the upstream returns thinking_signature_invalid": "Supprimer reasoning.encrypted_content et réessayer une fois lorsque le service en amont renvoie thinking_signature_invalid"
+ "Remove reasoning encrypted_content and retry once when the upstream returns thinking_signature_invalid": "Supprimer le champ reasoning.encrypted_content et réessayer une fois lorsque le service en amont renvoie thinking_signature_invalid"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Recover invalid reasoning signatures": "Récupérer les signatures de raisonnement invalides", | |
| "Remove reasoning encrypted_content and retry once when the upstream returns thinking_signature_invalid": "Supprimer reasoning.encrypted_content et réessayer une fois lorsque le service en amont renvoie thinking_signature_invalid" | |
| "Recover invalid reasoning signatures": "Récupérer les signatures de raisonnement invalides", | |
| "Remove reasoning encrypted_content and retry once when the upstream returns thinking_signature_invalid": "Supprimer le champ reasoning.encrypted_content et réessayer une fois lorsque le service en amont renvoie thinking_signature_invalid" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/i18n/locales/fr.json` around lines 5218 - 5219, Update the French
translation for “Remove reasoning encrypted_content and retry once when the
upstream returns thinking_signature_invalid” to explicitly preserve the nested
field path “reasoning.encrypted_content,” matching the actual field removed
during recovery.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
service/openai_reasoning_fallback_test.go (3)
29-29: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the exact SHA-256 conversation key.
The test only verifies that a hash exists, so regressions to the hash algorithm or input could pass. Compare the context key with the hex-encoded SHA-256 of
"conversation-a-first"to protect the cache-key contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/openai_reasoning_fallback_test.go` at line 29, Update the test around PrepareOpenAIResponsesReasoningInput to assert that the conversation context key exactly matches the hex-encoded SHA-256 digest of "conversation-a-first", replacing the current presence-only assertion while preserving the existing result checks.
66-66: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover an empty leading reasoning item.
The fixture never places an empty reasoning item before
"conversation-b-first", so a regression that selects the first reasoning item instead of the first non-emptyencrypted_contentwould pass. Add that case and retain the expected conversation-key behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/openai_reasoning_fallback_test.go` at line 66, Add a leading reasoning item with empty encrypted_content to the fixture used by PrepareOpenAIResponsesReasoningInput, followed by the existing non-empty reasoning item before "conversation-b-first". Keep the assertion verifying that the returned conversation key still corresponds to the first non-empty encrypted_content.
93-97: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the Redis-backed cache path.
All tests in this file disable Redis, so the hybrid-cache contract can regress in the Redis get/set path without failing this suite. Add a deterministic Redis-backed or mock-backend test covering learning and subsequent cache hits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/openai_reasoning_fallback_test.go` around lines 93 - 97, The tests in service/openai_reasoning_fallback_test.go only exercise the non-Redis cache path. Add a deterministic test alongside the existing reasoning fallback tests that keeps common.RedisEnabled enabled and uses a Redis-backed or mock backend to verify the first request learns/stores the result and a subsequent identical request is served from cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@service/openai_reasoning_fallback_test.go`:
- Line 29: Update the test around PrepareOpenAIResponsesReasoningInput to assert
that the conversation context key exactly matches the hex-encoded SHA-256 digest
of "conversation-a-first", replacing the current presence-only assertion while
preserving the existing result checks.
- Line 66: Add a leading reasoning item with empty encrypted_content to the
fixture used by PrepareOpenAIResponsesReasoningInput, followed by the existing
non-empty reasoning item before "conversation-b-first". Keep the assertion
verifying that the returned conversation key still corresponds to the first
non-empty encrypted_content.
- Around line 93-97: The tests in service/openai_reasoning_fallback_test.go only
exercise the non-Redis cache path. Add a deterministic test alongside the
existing reasoning fallback tests that keeps common.RedisEnabled enabled and
uses a Redis-backed or mock backend to verify the first request learns/stores
the result and a subsequent identical request is served from cache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a147631-9709-418e-8bf2-719b94901e78
📒 Files selected for processing (4)
controller/relay_reasoning_retry_test.gorelay/responses_handler.goservice/openai_reasoning_fallback.goservice/openai_reasoning_fallback_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- controller/relay_reasoning_retry_test.go
- relay/responses_handler.go
- service/openai_reasoning_fallback.go
1 participant
Important
📝 变更描述 / Description
OpenAI Responses 在
store: false场景会由客户端回传历史 reasoning item。生产请求中观察到上游返回thinking_signature_invalid：历史reasoning.encrypted_content无法由当前上游验证，而普通重试会原样发送相同内容，因此无法恢复。本改动仅对 OpenAI API 类型的 Responses 与 Responses Compact 生效，并且需要在渠道的“渠道额外设置”中主动开启“恢复无效推理签名”（默认关闭）：
thinking_signature_invalid时，用第一个非空reasoning.encrypted_content的 SHA-256 作为会话标识。encrypted_content，并保证一次不受全局重试次数限制的恢复重试。encrypted_content，避免跨渠道 fallback 使恢复失效。settingJSON，不新增数据库字段，也不需要数据库迁移。实现和说明由 AI 辅助整理，合并前请维护者复核。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
thinking_signature_invalid。✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
新增回归测试覆盖：开关关闭时不触发恢复、开关开启后的首次错误学习、强制恢复重试、跨渠道 fallback 后继续清理、后续独立请求继续尊重实际渠道开关、所有 reasoning 加密字段清理、后续缓存命中、首项哈希隔离，以及非 OpenAI/非 Responses 请求不触发降级。前端开关及其说明已同步到全部 7 个 locale。
Summary by CodeRabbit
reasoning.encrypted_contentand retrying once.thinking_signature_invalid, including a targeted retry-limit adjustment.